perf(semaphore): streamline permit distribution to waiting acquirers - #335

The explicit loop is easier to follow, and it preserves the first-panic behavior across batches.

Tests on Rust 1.86 and 1.98, Miri, and feature checks passed locally; the benchmarks improved too.

Small nit: the description still mentions PanicGuard. ^w^

PR description is updated now.

P.S. I told my agent to only generate a new summary without running gh command to edit PR description, but I forgot to update.

The back and forth catch_unwind/rewind seems putting more indirection.

Yes. Initially, I want to use the following pattern (written with Java pseudo code)

Throwable firstThrowable = null;
while (this.permits > 0) {
    var batch = this.pollAndReducePermits(this.wakers);
    if (batch.empty()) break;
    for (var waker : batch) {
        try {
            waker.wake();
        } catch (Throwable t) {
            if (firstThrowable == null) firstThrowable = t;
        }
    }
}
if (firstThrowable != null) throw firstThrowable;

However, since the wake_all method is still called directly in this PR, this PR actually looks like:

Throwable firstThrowable = null;
while (this.permits > 0) {
    var batch = this.pollAndReducePermits(this.wakers);
    if (batch.empty()) break;
    Throwable innerThrowable = null;
    for (var waker : batch) {
        try {
            waker.wake();
        } catch (Throwable t) {
            if (innerThrowable == null) innerThrowable = t;
        }
    }
    if (firstThrowable == null && innerThrowable != null) firstThrowable = innerThrowable;
}
if (firstThrowable != null) throw firstThrowable;

Then things become different.

Sorry to provide wrong insight.

I don't think so. Without looking deeper into the wake_all implementation, the current implementation is still not straightforward.

The current implementation in main branch

wake_all(std::iter::from_fn(|| {
    loop {
        if let Some(waker) = batch.next() { // [1]
            return Some(waker);
        }
        // [2]...
    }
}));

Initially, batch is empty, so it will start from [2] (step 1):

1. batch receives the wait nodes from waiters to the batch in each iteration until batch's stack storage is full. (that's what [2] does)
2. Poll the waker from the batch to wake_all, which calls the wake() method.
3. wake_all will repeat step 2 until any panic happens.
   1. If no panic happens in the batch, batch.next() in [1] will return None, then go to step 1 ([2]) again.
   2. Otherwise, wake_all will continue [2] in WakeRemaining::drop, but any panic will be ignored.

Eventually, the if rem == 0 { return None; } at the beginning of [2] will end the whole loop, where rem is guaranteed to be reduced to 0 in the rest implementation of [2].

I noticed other places like Shared::drop_sender also follows this wake_all pattern:

Anyway, it's still not straightforward and could be confusing.

I think the root cause for such indirection comes from the poorly designed wake_all method. We'd better express "resume-first-panic" clearly rather than coupling such panic handling pattern in a method whose name is "triggering a method on all elements in a collection".

That's why I've introduced PanicGuard before, but it might not be a good idea to apply it only in this single call.

Why this PR has better performance

This is interesting, so I told my agent to investigate for a while. In short, the improvement mainly comes from inlining, if a future LLVM inlines main's nested closure, the difference disappears.

1. should_unlink

In main, the should_unlink closure passed to unlink_first_waiter is emitted as its own function (WaitList::unlink_first_waiter::{{closure}} @ 0x1001564f8), and insert_permits_with_lock really calls it once per unlinked waiter:

10015a894: bl 0x1001564f8   ; predicate closure, inside the unlink loop

In this PR, the same predicate is inlined: just straight-line permit accounting and list updates. Reason: in main the predicate is nested inside the from_fn closure (insert_permits_with_lock::{{closure}}::{{closure}}), so LLVM outlines it. Cost ≈ 0.1 ns per waiter → ~24 ns at 256 waiters.

2. Lazy iterator

main routes the whole release through the inlined from_fn closure, and WakeRemaining::drop is outlined and carries that closure body. So the closure body is entered ~3× per release. That is a ~2 ns fixed cost, measurable with zero waiters, where no predicate is ever called:

release (no waiters):  main 21.1 / 21.8 ns   vs   current 19.8 / 19.8 ns

3. The refactor's extra catch_unwind is free on the success path.

No __rust_try call — only a cold landing pad (catch_unwind::cleanup) and the resume_unwind path. It costs code size (429 vs 335 instructions), not runtime, so it does not offset 1 and 2.

Net: ~2 ns fixed + ~0.1 ns/waiter, which is exactly the measured shape (1 waiter −2 ns, 2 waiters −3 ns, 32 within noise, 256 −24 ns).

@BewareMyPower I agree a new abstraction can be good to add. Just not co-exist and introduce one more indirection - replacing the current one is OK.